Skip to content

feat: allow visiting entire domain metadata #1384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: allow visiting entire domain metadata #1384

wants to merge 6 commits into from

Conversation

@samansmink
Copy link
Collaborator

What changes are proposed in this pull request?

Adding the ability to scan the entire domain metadata. This PR is in a way a follow up to #1342. I wanted to use the visit_string_map function from that PR, but then we would run into some ownership/lifetime issues: we would need to return a pointer to a kernel-allocated map

I thought the nicest solution would be to just create a separate visitor function which avoids any lifetime issues by scoping things to the visitor callback.

I also considered:

pub unsafe extern "C" fn visit_domain_metadata(
    snapshot: Handle<SharedSnapshot>,
    engine: Handle<SharedExternEngine>,
    engine_context: NullableCvoid,
    visitor: extern "C" fn(
        engine_context: NullableCvoid,
        map: &CStringMap,
    ),
)

Which would allow reusing the visit_string_map function and make this more in line with how engine is expected to consume other string maps, but I don't think the extra level of indirection really adds much and the visitor is very simple anyway.

This PR affects the following public APIs

  • Added new Snapshot::get_all_domain_metadata function
  • Added new ffi visit_domain_metadata to go over all domain metadata

How was this change tested?

  • Testing code added to existing kernel::snapshot::tests::test_domain_metadata test
  • Testing code added to existing ffi::domain_metadata::tests::test_domain_metadata test

@samansmink
Copy link
Collaborator Author

@OussamaSaoudi @zachschuermann can I bother you for a review here? :)

@github-actions github-actions bot added the breaking-change Change that require a major version bump label Oct 9, 2025
@codecov
Copy link

codecov bot commented Oct 9, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 95.55556% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.68%. Comparing base (2ec1462) to head (c57dfb2).

Files with missing lines Patch % Lines
kernel/src/snapshot.rs 86.66% 0 Missing and 2 partials ⚠️
ffi/src/domain_metadata.rs 98.38% 0 Missing and 1 partial ⚠️
kernel/src/actions/domain_metadata.rs 90.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1384      +/-   ##
==========================================
+ Coverage   84.65%   84.68%   +0.03%     
==========================================
  Files         115      115              
  Lines       29557    29644      +87     
  Branches    29557    29644      +87     
==========================================
+ Hits        25021    25104      +83     
  Misses       3329     3329              
- Partials     1207     1211       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

nicklan
nicklan reviewed Oct 24, 2025
View reviewed changes
Copy link
Collaborator

@nicklan nicklan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! I think we should slightly change the APIs, but basically this is good

ffi/src/domain_metadata.rs
Comment on lines +52 to +53
key: KernelStringSlice,
value: KernelStringSlice,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call these domain and configuration to match the protocol. we should probably also include if the domain is removed

kernel/src/actions/domain_metadata.rs
.remove(domain)
.map(|domain_metadata| domain_metadata.configuration))
}
pub(crate) fn all_domain_metadata_configuration(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add a newline above

ffi/src/lib.rs

pub(crate) type NullableCvoid = Option<NonNull<c_void>>;

#[derive(Default)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're not using CStringMap in this PR? Can we revert this change if it's not needed?

kernel/src/actions/domain_metadata.rs
pub(crate) fn all_domain_metadata_configuration(
log_segment: &LogSegment,
engine: &dyn Engine,
) -> DeltaResult<HashMap<String, String>> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a bit like an odd api. I'd prefer to return a Vec<DomainMetadata> where:

struct DomainMetadata {
  domain: String,
  configuration: String,
  removed: bool,
}

We already have exactly this type in actions/mod.rs but it's #[internal_api]. I think we should just make the get_all_domain_metadata in Snapshot also #[internal_api], we already enable that feature in the ffi crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicklan nicklan nicklan left review comments

@zachschuermann zachschuermann Awaiting requested review from zachschuermann

At least 2 approving reviews are required to merge this pull request.

Assignees

@samansmink samansmink

Labels

breaking-change Change that require a major version bump

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@samansmink @nicklan